Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct output redirection #929

Merged

Conversation

DominikKamp
Copy link
Contributor

This resolves #243 by restricting method redirectOutput() to the streams stdout and stderr.

@Joao-Dionisio
Copy link
Collaborator

Joao-Dionisio commented Nov 29, 2024

Hey hey, @DominikKamp, glad to see you here 😄

We had some problems with printing and people having weird locales (#830, #843). Do you think we might need to be concerned with this here as well?

Also, can you please add some tests?

@DominikKamp
Copy link
Contributor Author

Do you think we might need to be concerned with this here as well?

The function relayMessage() already gets an assembled string so that the locale should not matter here.

Also, can you please add some tests?

I do not really know, how the console output could be tested. But it could be tested that writeProblem() still creates a non-empty file after redirectOutput(). Where do unit tests go?

@Joao-Dionisio
Copy link
Collaborator

Joao-Dionisio commented Nov 29, 2024

Yeah, the output itself should be hard to test, but just that it's being printed to the correct place.

We have a bunch of test_XX.py files, so it's mostly a matter of seeing in which it makes more sense, defaulting to test_model.py. I think it makes sense to add it here. There's also the possibility of creating a new test file, if you think it is justified (it also needs to be prefixed by test_).

The test function needs to be prefixed by test_, and you can use some models we have stored in helpers/utils.py, in case you don't want to build a new one.

@DominikKamp
Copy link
Contributor Author

Tests added but please verify that the console output is still as expected with and without redirectOutput().

@DominikKamp
Copy link
Contributor Author

DominikKamp commented Nov 29, 2024

Before doing anything else, it seems like the mac pipeline has to be fixed because the github runner is not able to compile the SCIPOptSuite before eternity ends (why not use the precompiled version like in the Windows-test?).

@Opt-Mucca
Copy link
Collaborator

Changes look good. I will test before merging.

@Opt-Mucca
Copy link
Collaborator

This does fix the issue on writeProblem. It also seems to help with #896

@Opt-Mucca Opt-Mucca enabled auto-merge December 3, 2024 09:26
@Opt-Mucca
Copy link
Collaborator

Let's wait until #933 is merged as then the pipelines can run faster

@Opt-Mucca Opt-Mucca merged commit d05b049 into master Dec 3, 2024
16 checks passed
@Joao-Dionisio Joao-Dionisio deleted the 243-fix-write-methods-dont-work-after-redirectoutput branch January 16, 2025 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"write" methods don't work after redirectOutput()
3 participants